Refactor App and SubApp internals for better separation
#9202
Conversation
maniwani
added
C-Code-Quality
|
Example |
maniwani
force-pushed
the
refactor-app-and-sub-app
branch
3 times, most recently
from
974bf3a
to
a0f986a
Compare
|
Example |
maniwani
force-pushed
the
refactor-app-and-sub-app
branch
from
a0f986a
to
9fcb6e4
Compare
|
Example |
maniwani
force-pushed
the
refactor-app-and-sub-app
branch
from
9fcb6e4
to
db24bc1
Compare
| sub_apps: SubApps { | ||
| main: SubApp::new(), | ||
| sub_apps: HashMap::new(), | ||
| }, |
There was a problem hiding this comment.
What's the SubApps struct for? How come these fields aren't just stored directly under App?
There was a problem hiding this comment.
Yeah I understand the motivation behind this PR. I'm specifically asking about these two fields.
Why is it defined like this
struct App {
sub_apps: SubApps,
...
}
struct SubApps {
main: SubApp,
sub_apps: HashMap<AppLabelId, SubApp>,
}instead of like this?
struct App {
main: SubApp,
sub_apps: HashMap<AppLabelId, SubApp>,
...
}There was a problem hiding this comment.
It's so you don't have to write the contents of SubApps::update by hand in each runner (it replaces App::update since runners basically can't use that after #9122). If you'd like to get rid of the sub_apps.sub_apps, please suggest a different composition that still avoids boilerplate.
There was a problem hiding this comment.
Oh that makes sense. If SubApps is meant to be used externally, then there should probably be a way of accessing it.
There was a problem hiding this comment.
Would this be better?
struct App {
main: SubApp,
sub_apps: SubApps,
...
}
#[derive(Deref, DerefMut)]
struct SubApps(HashMap<AppLabelId, SubApp>);
impl SubApps {
fn extract(&mut self, main: &mut SubApp) {
...
}
...
}There was a problem hiding this comment.
Excluding the main app from the sub-apps collection does make more sense to me, yeah.
There was a problem hiding this comment.
What about pushing things in the other direction and try to make the main app less special?
struct App {
active_app_label: AppLabel,
sub_apps: SubApps, ...
}
impl SubApps {
fn extract(&mut self, main_label: AppLabel) {
let main_app = self.0.get(main_label);
...
}
}
maniwani
force-pushed
the
refactor-app-and-sub-app
branch
2 times, most recently
from
5683a8b
to
4823a8c
Compare
|
|
||
| /// Runs [`Plugin::finish`] for each plugin. | ||
| pub fn finish(&mut self) { | ||
| let plugins = std::mem::take(&mut self.plugins); |
There was a problem hiding this comment.
The logic for a lot of these feels like it should be inverted. i.e. We should just be passing SubApp into the plugin methods. But rename SubApp -> App and rename the super app into something else like Bevy::new(). That would get rid of most if not all of the uses of run_as_app.
edit: actually on second thought this is might be controversial enough that it should be pushed off to a separate pr. Render plugins will need to be aware of which app they're being added to. Which will require some major changes. But the data flow is definitely a little wonky now since SubApps own plugins, but I'm fine with fixing in another pr and merging as is to unblock fixing nonsend resources.
There was a problem hiding this comment.
That's a bigger change, yeah. Plugins can currently see everything and the various render-related plugins do currently make use of that to add things to both the main app and the rendering sub-app.
There was a problem hiding this comment.
Could that be achieved with an API call? something like:
impl Plugin for MyPlugin {
fn build(&mut sub_app) {
let render_app = sub_app.get_sibling(RenderApp);
}
}| sub_apps: SubApps { | ||
| main: SubApp::new(), | ||
| sub_apps: HashMap::new(), | ||
| }, |
There was a problem hiding this comment.
What about pushing things in the other direction and try to make the main app less special?
struct App {
active_app_label: AppLabel,
sub_apps: SubApps, ...
}
impl SubApps {
fn extract(&mut self, main_label: AppLabel) {
let main_app = self.0.get(main_label);
...
}
}
maniwani
force-pushed
the
refactor-app-and-sub-app
branch
2 times, most recently
from
afe77f6
to
1ee3b95
Compare
There was a problem hiding this comment.
I like this architecture a lot better, and the code / doc quality is solid.
In particular, no longer storing App nested indefinitely in itself is really important for architectural (and UX) clarity. I agree with Hymm, moving this forward is important and this is definitely a step in the right direction.
alice-i-cecile
added
the
S-Ready-For-Final-Review
maniwani
force-pushed
the
refactor-app-and-sub-app
branch
2 times, most recently
from
d44d1c5
to
d760588
Compare
maniwani
force-pushed
the
refactor-app-and-sub-app
branch
from
2edfdfe
to
2e4e372
Compare
alice-i-cecile
removed
the
M-Needs-Release-Note
- [9202](bevyengine/bevy#9202) changed world access to functions. [relevent line](https://github.com/bevyengine/bevy/pull/9202/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97fR640) - This also surfaced [12655](bevyengine/bevy#12655) which removed `Into<AssetId<T>>` for `Handle<T>`. using a reference or .id() is the solution here.
# Objective - Let `init_non_send_resource` take `FromWorld` values again, not only `Default` - This reverts an unintended breaking change introduced in #9202 ## Solution - The resource initialized with `init_non_send_resource` requires `FromWorld` again
# Objective - Let `init_non_send_resource` take `FromWorld` values again, not only `Default` - This reverts an unintended breaking change introduced in #9202 ## Solution - The resource initialized with `init_non_send_resource` requires `FromWorld` again
Addresses Bevy [9202](bevyengine/bevy#9202)
Addresses Bevy[9202](bevyengine/bevy#9202)
Addresses Bevy [9202](bevyengine/bevy#9202)
Addresses Bevy[9202](bevyengine/bevy#9202)
Addresses Bevy [9202](bevyengine/bevy#9202)
* Update to 0.14.0-rc.2 * [12997](bevyengine/bevy#12997): rename `multi-threaded` to `multi_threaded` * RenderAssets<Image> is now RenderAssets<GpuImage> Implemented in [12827](bevyengine/bevy#12827) * FloatOrd is now in bevy_math implemented in [12732](bevyengine/bevy#12732) * convert Transparent2d::dynamic_offset to extra_index [12889](bevyengine/bevy#12889) Gpu Frustum Culling removed the dynamic_offset of Transparent2d and it became `extra_index` with the special value `PhaseItemExtraIndex::NONE`, which indicates the `None` that was here previously * RenderPhase<Transparent2d> -> ViewSortedRenderPhases<Transparent2d> [12453](https://github.com/StarArawn/bevy_ecs_tilemap/pull/bevyengine/bevy#12453): Render phases are now binned or sorted. Following the changes in the `mesh2d_manual` [example](https://github.com/bevyengine/bevy/blob/ecdd1624f302c5f71aaed95b0984cbbecf8880b7/examples/2d/mesh2d_manual.rs#L357-L358): use the `ViewSortedRenderPhases` resource. * get_sub_app_mut is now an Option in [9202](https://github.com/StarArawn/bevy_ecs_tilemap/pull/bevyengine/bevy/pull/9202) SubApp access has changed * GpuImage::size f32 -> u32 via UVec2 [11698](bevyengine/bevy#11698) changed `GpuImage::size` to `UVec2`. Right above this, `Extent3d` does the same thing, so I'm taking a small leap and assuming can `as`. * GpuMesh::primitive_topology -> key_bits/BaseMeshPipeline [12791](bevyengine/bevy#12791) the `primitive_topology` field on `GpuMesh` was removed in favor of `key_bits` which can be constructed using `BaseMeshPipeline::from_primitive_topology` * RenderChunk2d::prepare requires &mut MeshVertexBufferLayouts now [12216](bevyengine/bevy#12216) introduced an argument `&mut MeshVertexBufferLayouts` to `get_mesh_vertex_buffer_layout`, which bevy_ecs_tilemap calls in `RenderChunk2d::prepare` * into_linear_f32 -> color.0.linear().to_f32_array(), [12163](bevyengine/bevy#12163) bevy_color was created and Color handling has changed. Specifically Color::as_linear_rgba_f32 has been removed. LinearRgba is now its own type that can be accessed via [`linear()`](https://docs.rs/bevy/0.14.0-rc.2/bevy/color/enum.Color.html#method.linear) and then converted. * Must specify type of VisibleEntities when accessing [12582](bevyengine/bevy#12582) divided `VisibleEntities` into separate lists. So now we have to specify which kind of entity we want. I think we want the Mesh here, and I think we can get rid of the `.index` calls on Entity since Entity [already compares bits](https://docs.rs/bevy_ecs/0.14.0-rc.2/src/bevy_ecs/entity/mod.rs.html#173) for optimized codegen purposes. Waiting to do that until the other changes are in though so as to not change functionality until post-upgrade. * app.world access is functions now - [9202](bevyengine/bevy#9202) changed world access to functions. [relevent line](https://github.com/bevyengine/bevy/pull/9202/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97fR640) - This also surfaced [12655](bevyengine/bevy#12655) which removed `Into<AssetId<T>>` for `Handle<T>`. using a reference or .id() is the solution here. * We don't need `World::cell`, and it doesn't exist anymore In [12551](bevyengine/bevy#12551) `WorldCell` was removed. ...but it turns out we don't need it or its replacement anyway. * examples error out unless this bevy bug is addressed with these features being added bevyengine/bevy#13728 * check_visibility is required for the entity that is renderable As a result of [12582](bevyengine/bevy#12582) `check_visibility` must be implemented for the "renderable" tilemap entities. Doing this is trivial by taking advantage of the existing `check_visibility` type arguments, which accept a [`QF: QueryFilter + 'static`](https://docs.rs/bevy/0.14.0-rc.2/bevy/render/view/fn.check_visibility.html). The same `QueryFilter`` is used when checking `VisibleEntities`. I've chosen `With<TilemapRenderSettings` because presumably if the entity doesn't have a `TilemapRenderSettings` then it will not be rendering, but this could be as sophisticated or simple as we want. For example `WithLight` is currently implemented as ```rust pub type WithLight = Or<(With<PointLight>, With<SpotLight>, With<DirectionalLight>)>; ``` * view.view_proj -> view.clip_from_world [13289](bevyengine/bevy#13489) introduced matrix naming changes, including `view_proj` which becomes `clip_from_world` * color changes to make tests runnable * clippy fix * Update Cargo.toml Co-authored-by: Rob Parrett <[email protected]> * Update Cargo.toml Co-authored-by: Rob Parrett <[email protected]> * final clippy fixes * Update Cargo.toml Co-authored-by: Rob Parrett <[email protected]> * Simplify async loading in ldtk/tiled helpers See Bevy #12550 * remove second allow lint * rc.3 bump * bump version for major release * remove unused features --------- Co-authored-by: Rob Parrett <[email protected]>
Addresses Bevy [9202](bevyengine/bevy#9202)
Addresses Bevy[9202](bevyengine/bevy#9202)
Objective
This is a necessary precursor to #9122 (this was split from that PR to reduce the amount of code to review all at once).
Moving
!Sendresource ownership toAppwill make it unambiguously!Send.SubAppmust beSend, so it can't wrapApp.Solution
Refactor
AppandSubAppto not have a recursive relationship. SinceSubAppno longer wrapsApp, once!Sendresources are moved out ofWorldand intoApp,SubAppwill become unambiguouslySend.There could be less code duplication between
AppandSubApp, but that would breakAppmethod chaining.Changelog
SubAppno longer wrapsApp.Appfields are no longer publicly accessible.Appcan no longer be converted into aSubApp.SubAppinstead of anApp.Migration Guide
SubApp::new().Appcan no longer convert intoSubApp.App, you may want to implement it forSubAppas well.app.worlddirectly, you now have to useapp.world()andapp.world_mut().App::sub_appnow returns&SubApp.App::sub_app_mutnow returns&mut SubApp.App::get_sub_appnow returnsOption<&SubApp>.App::get_sub_app_mutnow returnsOption<&mut SubApp>.